Skip to content

Conversation

@rodrigobr-msft
Copy link
Contributor

@rodrigobr-msft rodrigobr-msft commented Oct 23, 2025

This pull request refactors and improves the storage and error handling infrastructure for the Microsoft Agents activity and hosting core libraries. The main changes involve modularizing utility functions, introducing new storage namespace abstractions, and replacing custom storage client logic with standardized wrappers for managing state. These updates simplify code maintenance, enhance clarity, and make state management more robust and consistent.

Error handling and utility improvements:

  • Added _raise_if_none and _raise_if_falsey helper functions for input validation, and moved load_configuration_from_env to a new _utils module for better organization. [1] [2] [3] [4]

Storage abstraction and namespace improvements:

  • Introduced _Namespaces and storage wrapper classes (_ItemNamespace, _StorageNamespace, _ItemStorage) to standardize namespacing and item management in storage operations. These are now exported in the storage package for use across the codebase. [1] [2]
  • Updated the OAuth authorization logic to use _ItemNamespace for sign-in state management, replacing custom key generation and CRUD methods with standardized wrapper calls. [1] [2] [3] [4] [5] [6] [7] [8]

Removal of legacy code and simplification:

  • Removed the _FlowStorageClient class and its usage, replacing it with _ItemNamespace and standardized storage wrappers for OAuth flow state management. [1] [2] [3] [4] [5] [6]

Internal API consistency:

  • Refactored handler initialization to use _StorageNamespace for isolating storage per handler, improving separation of concerns and consistency.

Documentation and code clarity:

Copilot AI review requested due to automatic review settings October 23, 2025 15:27
@rodrigobr-msft rodrigobr-msft changed the title Users/robrandao/storage wrappers Internal utilities and wrappers for Storage protocols Oct 23, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces storage wrapper classes to improve the modularity and organization of storage operations in the microsoft-agents framework. The changes include new wrapper implementations for namespaced and item-based storage access patterns, along with corresponding test coverage.

Key changes:

  • Added _StorageNamespace, _ItemStorage, and _ItemNamespace wrapper classes to provide namespace-based key isolation and single-item storage operations
  • Refactored authorization code to use the new storage wrappers instead of direct storage access
  • Updated imports and type hints across the codebase for consistency
  • Enhanced error handling by replacing pass with raise NotImplementedError() in abstract methods

Reviewed Changes

Copilot reviewed 28 out of 33 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/hosting_core/storage/wrappers/test_storage_namespace.py Comprehensive test suite for _StorageNamespace wrapper
tests/hosting_core/storage/wrappers/test_item_storage.py Test coverage for single-item storage operations
tests/hosting_core/storage/wrappers/test_item_namespace.py Tests for combined namespace and item storage functionality
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/_wrappers/_storage_namespace.py Implementation of namespace-based storage key prefixing
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/_wrappers/_item_storage.py Single-item storage operation wrapper
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/_wrappers/_item_namespace.py Combines namespace and item storage patterns
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/storage.py Updated abstract methods to raise NotImplementedError instead of pass; renamed AsyncStorageBase to _AsyncStorageBase
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/memory_storage.py Fixed scoping issue by moving result dictionary initialization
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/app/oauth/authorization.py Refactored to use new storage wrappers for sign-in state management
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/app/oauth/_handlers/_user_authorization.py Updated to use _ItemNamespace instead of deleted _FlowStorageClient
libraries/microsoft-agents-activity/microsoft_agents/activity/_utils/_error_handling.py New error handling utilities for parameter validation
libraries/microsoft-agents-hosting-core/microsoft_agents/hosting/core/storage/_namespaces.py Centralized storage namespace constants

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

TranscriptMemoryStore,
)
from ._wrappers import (
_StorageNamespace
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing comma after '_StorageNamespace' on line 17 causes a syntax error when followed by '_ItemNamespace' on line 18.

Suggested change
_StorageNamespace
_StorageNamespace,

Copilot uses AI. Check for mistakes.
"aud"
]

namespace = _Namespaces._USER_AUTHORIZATION.format(
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reference to '_Namespaces._USER_AUTHORIZATION' is incorrect. The attribute in _namespaces.py is defined as 'USER_AUTHORIZATION' (without underscore prefix), so this should be '_Namespaces.USER_AUTHORIZATION'.

Suggested change
namespace = _Namespaces._USER_AUTHORIZATION.format(
namespace = _Namespaces.USER_AUTHORIZATION.format(

Copilot uses AI. Check for mistakes.

namespace = _Namespaces._USER_AUTHORIZATION.format(
channel_id=channel_id,
user_id=user_id,
Copy link

Copilot AI Oct 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The format string expects 'from_property_id' as a parameter (line 8 in _namespaces.py), but 'user_id' is being passed here. This will cause a KeyError when attempting to format the namespace string.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants